Skip to content

Add a basic benchmark#593

Open
simonmarty wants to merge 17 commits into
aws:mainfrom
simonmarty:benchmark
Open

Add a basic benchmark#593
simonmarty wants to merge 17 commits into
aws:mainfrom
simonmarty:benchmark

Conversation

@simonmarty

@simonmarty simonmarty commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

Description

Why is this change being made?

  1. We don't have a way to detect performance regressions in the CSI driver.

What is changing?

  1. Added a really basic set of benchmarks using Go's built-in benchmark tooling and a little github action to compare the benchmark results to the baseline.
  2. Warning: The GitHub action workflow was almost entirely written by AI. I'm still investigating whether the way it's outputting benchmark diffs is the best way to do things.

Related Links

  • Issue #, if available:

Testing

How was this tested?

  1. Tested on my fork.

When testing locally, provide testing artifact(s):

Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why:
  • Integration tests pass locally
    If not, why: Benchmark doesn't run over the network, no changes to integ tests.
  • I have updated integration tests (if needed)
    If not, why:
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why:
  • I have updated README/documentation (if needed)
    If not, why:
  • I have clearly called out breaking changes (if any)
    If not, why: No breaking changes, this is purely incremental.

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@simonmarty simonmarty requested a review from a team as a code owner March 27, 2026 17:01
@codecov

codecov Bot commented Mar 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.84%. Comparing base (0d35d4f) to head (55214f1).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #593   +/-   ##
=======================================
  Coverage   60.84%   60.84%           
=======================================
  Files          11       11           
  Lines         710      710           
=======================================
  Hits          432      432           
  Misses        261      261           
  Partials       17       17

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simonmarty
simonmarty commented Mar 27, 2026
View reviewed changes
Comment thread .github/workflows/bench.yml Outdated
@simonmarty simonmarty enabled auto-merge (squash) April 1, 2026 22:58
@simonmarty simonmarty added safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. labels Apr 2, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 2, 2026
reyhankoyun
reyhankoyun reviewed Apr 6, 2026
View reviewed changes
Comment thread .github/workflows/bench.yml Outdated
Comment thread server/benchmark_test.go Outdated
Comment thread server/benchmark_test.go
@reyhankoyun reyhankoyun added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 6, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 13, 2026
@simonmarty

simonmarty commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

Latest successful benchmark comparison here: https://github.com/simonmarty/secrets-store-csi-driver-provider-aws/actions/runs/24364877168

@reyhankoyun reyhankoyun added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 13, 2026
reyhankoyun
reyhankoyun reviewed Apr 13, 2026
View reviewed changes
Comment thread server/benchmark_test.go Outdated
Comment thread .github/workflows/bench.yml
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 14, 2026
@reyhankoyun reyhankoyun added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 14, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 14, 2026
@reyhankoyun reyhankoyun added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 14, 2026
@reyhankoyun reyhankoyun added safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. and removed safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. labels Apr 14, 2026
@reyhankoyun reyhankoyun removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 14, 2026
@simonmarty simonmarty added safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. and removed safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. labels Apr 14, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 15, 2026
reyhankoyun
reyhankoyun reviewed Apr 16, 2026
View reviewed changes
Comment thread .github/workflows/bench.yml
uses: actions/cache/restore@v5
with:
path: bench-baseline.json
key: benchmark-baseline-${{ runner.os }}-${{ github.event.pull_request.base.sha }}

@reyhankoyun reyhankoyun Apr 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you pointed out on the agent, cache entries are immutable and the exact-key restore here might silently miss when the PR base SHA doesn't match a saved key. The GitHub docs recommend using a unique-per-run key with a restore-keys prefix fallback in a single actions/cache step, which lets you drop the separate save step too. I just fixed this on the agent repo

@simonmarty simonmarty Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache entry for the base sha should always exist since we run the action on every push.

@reyhankoyun reyhankoyun Apr 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay to ship as it'll work in the common case, and a miss would just mean no comparison (not a failure). The github-action-benchmark step will still run, it just won't have a baseline to compare against. Non-blocking.

reyhankoyun
reyhankoyun previously approved these changes Apr 20, 2026
View reviewed changes
@simonmarty simonmarty added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Apr 20, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label May 28, 2026
@simonmarty simonmarty added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label May 28, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Jun 4, 2026
@simonmarty simonmarty added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Jun 4, 2026
@github-actions github-actions Bot removed the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Jun 18, 2026
@simonmarty simonmarty added the safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on. label Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reyhankoyun reyhankoyun reyhankoyun left review comments
@lohdipak lohdipak Awaiting requested review from lohdipak

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

safe-to-test Pull Request has been manually reviewed and deemed to be safe to run integration tests on.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@simonmarty @reyhankoyun